Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Framework: Stop forcing CMake to pass to -fPIE on all compilations, as this leads to link failures with Intel 19 #10984

Closed
wants to merge 1 commit into from

Conversation

PhilMiller
Copy link
Contributor

@PhilMiller PhilMiller commented Sep 2, 2022

@trilinos/framework @bartlettroscoe

Fixes #10982

Motivation

Prevent dynamic linking failures like this:

[18/654] Linking CXX executable packages/panzer/dof-mgr/test/fe_assembly/PanzerDofMgr_Test_FE_Assembly_HEX.exe
FAILED: packages/panzer/dof-mgr/test/fe_assembly/PanzerDofMgr_Test_FE_Assembly_HEX.exe 
: && /projects/sems/install/rhel7-x86_64/sems/compiler/intel/19.0.5/openmpi/1.10.1/bin/mpicxx -D_GLIBCXX_USE_CXX11_ABI=0     -O3 -DNDEBUG -mkl packages/panzer/dof-mgr/test/fe_assembly/CMakeFiles/PanzerDofMgr_Test_FE_Assembly_HEX.dir/test_fe_assembly_HEX.cpp.o packages/panzer/dof-mgr/test/fe_assembly/CMakeFiles/PanzerDofMgr_Test_FE_Assembly_HEX.dir/__/cartesian_topology/CartesianConnManager.cpp.o -o packages/panzer/dof-mgr/test/fe_assembly/PanzerDofMgr_Test_FE_Assembly_HEX.exe  -Wl,-rpath,/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/panzer/dof-mgr/src:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/intrepid2/src:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/shards/src:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/phalanx/src:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/sacado/src:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/panzer/core/src:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/tpetra/core/ext:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/tpetra/core/inout:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/tpetra/core/src:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/tpetra/core/compat:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/tpetra/tsqr/src:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/kokkos-kernels/src:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/kokkos/algorithms/src:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/kokkos/containers/src:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/epetra/src:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/teuchos/numerics/src:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/teuchos/remainder/src:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/teuchos/kokkoscomm/src:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/teuchos/comm/src:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/teuchos/kokkoscompat/src:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/teuchos/parameterlist/src:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/teuchos/parser/src:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/teuchos/core/src:/scratch/pbmille/trilinos/build/INTEL-19.0.5-RELEASE-SERIAL-SHARED/packages/kokkos/core/src  packages/panzer/dof-mgr/src/libpanzer-dof-mgr.so.13.5  packages/intrepid2/src/libintrepid2.so.13.5  packages/shards/src/libshards.so.13.5  packages/phalanx/src/libphalanx.so.13.5  packages/sacado/src/libsacado.so.13.5  packages/panzer/core/src/libpanzer-core.so.13.5  packages/tpetra/core/ext/libtpetraext.so.13.5  packages/tpetra/core/inout/libtpetrainout.so.13.5  packages/tpetra/core/src/libtpetra.so.13.5  packages/tpetra/core/compat/libtpetraclassic.so.13.5  packages/tpetra/tsqr/src/libkokkostsqr.so.13.5  packages/kokkos-kernels/src/libkokkoskernels.so.13.5  packages/kokkos/algorithms/src/libkokkosalgorithms.so.13.5  packages/kokkos/containers/src/libkokkoscontainers.so.13.5  packages/epetra/src/libepetra.so.13.5  packages/teuchos/numerics/src/libteuchosnumerics.so.13.5  packages/teuchos/remainder/src/libteuchosremainder.so.13.5  packages/teuchos/kokkoscomm/src/libteuchoskokkoscomm.so.13.5  packages/teuchos/comm/src/libteuchoscomm.so.13.5  packages/teuchos/kokkoscompat/src/libteuchoskokkoscompat.so.13.5  packages/teuchos/parameterlist/src/libteuchosparameterlist.so.13.5  packages/teuchos/parser/src/libteuchosparser.so.13.5  packages/teuchos/core/src/libteuchoscore.so.13.5  packages/kokkos/core/src/libkokkoscore.so.13.5  -ldl && :
ld: packages/panzer/dof-mgr/test/fe_assembly/CMakeFiles/PanzerDofMgr_Test_FE_Assembly_HEX.dir/test_fe_assembly_HEX.cpp.o(.text+0xc38c): unresolvable R_X86_64_TPOFF32 relocation against symbol `_ZN6Kokkos4Impl22SharedAllocationRecordIvvE18t_tracking_enabledE'
ld: final link failed: Nonrepresentable section on output

Stakeholder Feedback

Testing

I manually tested this with Intel 19.0.5 ATDM compiler configuration, and confirmed that EMPIRE built on top of Trilinos with this modification built and ran successfully.

Automatic testing would be dependent on having a system and target suitably configured.

…ns, as this leads to link failures with Intel 19
Copy link
Member

@bartlettroscoe bartlettroscoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of removing support for setting -fpic, lets just change the default from ON to OFF. See comment below.

I can make the change and push a new commit if you like.

Comment on lines -220 to -223
# FPIC
IF (ATDM_FPIC)
SET(CMAKE_POSITION_INDEPENDENT_CODE ON CACHE BOOL "Build targets with position independent code")
ENDIF()
Copy link
Member

@bartlettroscoe bartlettroscoe Sep 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PhilMiller, I just remembered that some SPARC builds expect fpic to be on for its shared builds. Therefore, to remove this may break SPARC's usage.

How about we change the default for fpic from on to off and require those shared builds that want to set fpic to have to on have to include the keyword fpic the build name? Then the SPARC shared builds that need fpic could just add the keyword fpic to their build names.

So we would update:

Then we could leave this logic here?

I think to change the default we just need to edit the following files:

Copy link
Contributor Author

@PhilMiller PhilMiller Sep 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which does SPARC actually want, -fPIC so it can link objects into shared libraries, or -fPIE to link objects into Position-Independent Executables? PIE is generally only desirable for relocatable loading to support ASLR in a server process, or for very niche use cases in which other processes will load an executable image as a dynamic library.

The ATDM_FPIC setting is currently asking CMake to provide PIE, according to https://cmake.org/cmake/help/latest/prop_tgt/POSITION_INDEPENDENT_CODE.html#prop_tgt:POSITION_INDEPENDENT_CODE and its observed behavior, which besides breaking builds, also hurts performance if it's used in circumstances where it's not actually necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rppawlo asked me to weigh in. SPARC wants Trilinos static libraries so that we can link them into either shared or static builds of our application executables.

@PhilMiller
Copy link
Contributor Author

The thing is, the underlying CMake variable doesn't control setting -fPIC, it effectively asks it to set -fPIE. If one wants PIC, BUILD_SHARED_LIBS takes care of that. If that's not being set, it should be.

@trilinos-autotester
Copy link
Contributor

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

@bartlettroscoe
Copy link
Member

The thing is, the underlying CMake variable doesn't control setting -fPIC, it effectively asks it to set -fPIE. If one wants PIC, BUILD_SHARED_LIBS takes care of that. If that's not being set, it should be.

Yes, when shared libraries are built (i.e. BUILD_SHARED_LIBS=ON), PIC code is automatically enabled (by the compiler). And use, BUILD_SHARED_LIBS is being set by the shared and static build-name keywords. See:

and:

@jjellio
Copy link
Contributor

jjellio commented Sep 4, 2022

Maybe use:
https://cmake.org/cmake/help/latest/policy/CMP0083.html

and try using

include(CheckPIESupported)
check_pie_supported()

Or could we just set CMAKE_<lang>_LINK_NO_PIE_SUPPORTED:BOOL=TRUE
From
https://cmake.org/cmake/help/latest/module/CheckPIESupported.html

@github-actions
Copy link

github-actions bot commented Sep 9, 2023

This Pull Request has been marked for closure due to inactivity.
Because of the changing nature of the Trilinos source due to active development, a pull request with no activity for 365 days is considered to be abandoned and will be automatically closed after 30 additional days of inactivity from when it was marked inactive.
If this should be kept open, please post a comment and/or remove the label MARKED_FOR_CLOSURE to reset the inactivity timer.
If it is ok for this pull request to be closed, feel free to go ahead and close it. Please do not add any comments or change any labels or otherwise touch this issue unless your intention is to reset the inactivity counter for an additional year.

@github-actions github-actions bot added the MARKED_FOR_CLOSURE Issue or PR is marked for auto-closure by the GitHub Actions bot. label Sep 9, 2023
@PhilMiller
Copy link
Contributor Author

AFAIK, this hasn't changed. Closure on this one should come after Intel classic is out of use, or testing shows it's fixed.

@github-actions github-actions bot removed the MARKED_FOR_CLOSURE Issue or PR is marked for auto-closure by the GitHub Actions bot. label Sep 10, 2023
@trilinos-autotester
Copy link
Contributor

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

@sebrowne sebrowne changed the title #10982: Stop forcing CMake to pass to -fPIE on all compilations, as this leads to link failures with Intel 19 Framework: Stop forcing CMake to pass to -fPIE on all compilations, as this leads to link failures with Intel 19 Sep 16, 2024
@sebrowne
Copy link
Contributor

sebrowne commented Sep 16, 2024

The ATDM configure/build system is no longer in-use. The changes to config-specs.ini are inappropriate, as they remove the ability to build with PIC at all. A more-acceptable approach would be to change the configurations to specify no-pic, but even then, all this affects is the PR testing configurations. None of these changes should affect how users build the code as far as I'm aware (unless they're using the Trilinos ini files). Closing, but leaving issue open for additional discussion.

@sebrowne sebrowne closed this Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants